build: untangle UNITTESTS and DEBUGBUILD macros
#13694
Conversation
vszakats
changed the title
Before this patch, building unit tests required `CURLDEBUG` to be set either via `ENABLE_DEBUG=ON` or `ENABLE_CURLDEBUG=ON`. After fixing the build issues in curl#13694, the above requirement is no longer necessary as a workaround. This patch makes unit tests build unconditionally. Depends-on: curl#13694 Ref: curl#13697 Follow-up to 39e7c22 curl#11446 Closes #xxxxx
Before this patch, building unit tests (= the `testdeps` target) required `-DCURLDEBUG` be set either via `ENABLE_DEBUG=ON` or `ENABLE_CURLDEBUG=ON`. After fixing the build issues in curl#13694, the above requirement is no longer necessary as a workaround. This patch makes unit tests build unconditionally. Depends-on: curl#13694 (fix build issues) Depends-on: curl#13697 (fix unit test issue revealed by Old Linux CI job) Follow-up to 39e7c22 curl#11446 Closes curl#13698
Based on failing then fixed jobs in curl#13689
| @@ -54,7 +54,7 @@ | |||
| #define MAX_HSTS_DATELENSTR "64" | |||
| #define UNLIMITED "unlimited" | |||
|
|
|||
| #ifdef DEBUGBUILD | |||
| #if defined(DEBUGBUILD) || defined(UNITTESTS) | |||
There was a problem hiding this comment.
Why is this needed?
The unit tests assume that DEBUGBUILD is defined as a prereq for running. This change seems to say that you can build with UNITTESTS without DEBUGBUILD ?
There was a problem hiding this comment.
Unit tests (and any other test) are building fine without either DEBUGBUILD or CURLDEBUG. (That is after fixing the places where DEBUGBUILD was used to enable functions for unit tests.)
This is an outlier case where the deltatime variable of a debug feature (CURL_TIME) is actually required for building a unit test.
I wish we could drop the distinction completely and just use one of them. I can't remember the difference, I don't think anyone uses them separately. Am I wrong? |
They are not the same thing, but their names don't make it trivial to figure out which does what exactly:
In CMake, Yet, they are separate and possible to use separately, with quite a bit of breakage around the edges. The goal of this patch isn't really to untangle these two from each other, but to fix to use the Merging This PR is an attempt to fix all existing build combinations. |
|
There seem to be a few suspicious cases, where I think we might consider renaming With the list of related open PRs in the air, I'd prefer merging these first, then continue tackling more. Two PRs are blocked by this one: #13705 and #13698. |
vszakats
changed the title
vszakats
changed the title
UNITTESTS and DEBUGBUILD macros
vszakats
changed the title
UNITTESTS and DEBUGBUILD macrosUNITTESTS and DEBUGBUILD macros
As Viktor mentions above, yes they are separate because CURLDEBUG is (supposed to be) the guard for the trackmemory feature. Typically I run debug builds of curl with DEBUGBUILD but not CURLDEBUG.
I'm with you on the first part (maybe something shorter?) but not the second part. Renaming DEBUGBUILD to CURL_DEBUG is confusing to me since we've been using CURLDEBUG for memory tracking. |
Indeed, I agree. If we want to rename them (or one of them), we can brainstorm for new names. Starting with Anyway, after tidying this up, I think it will be a little bit harder to make a mistake by copy-pasting existing code or looking up existing uses. As for edit: merging |
|
I can see UNITTESTS being dependent on DEBUGBUILD but not the other way around |
Before this patch `ENABLE_DEBUG=ON` always enabled the TrackMemory (aka `ENABLE_CURLDEBUG=ON`) feature, but required the `Debug` CMake configration to actually enable curl debug features (aka `-DDEBUGBUILD`). Curl debug features do not require compiling with C debug options. This also made enabling debug features unintuitive and complicated to use. Due to other issues (subject to PR #13694) it also caused an error in default (and `Release`/`MinSizeRel`/`RelWithDebInfo`) configs, when building the `testdeps` target: ``` ld: CMakeFiles/unit1395.dir/unit1395.c.o: in function `test': unit1395.c:(.text+0x1a0): undefined reference to `dedotdotify' ``` Ref: https://github.com/curl/curl/actions/runs/9037287098/job/24835990826#step:3:2483 Fix it by always defining `DEBUGBUILD` when setting `ENABLE_DEBUG=ON`. Decoupling this option from the selected CMake configuration. Note that after this patch `ENABLE_DEBUG=ON` unconditionally enables curl debug features. These features are insecure and unsuited for production. Make sure to omit this option when building for production in default, `Release` (and other not-`Debug`) modes. Also delete a workaround no longer necessary in GHA CI jobs. Ref: 1a62b6e (2015-03-03) Ref: #13583 Closes #13592
vszakats
force-pushed
the
debug-untangle-2
branch
from
880859e
to
ce83d3c
Compare
vszakats
force-pushed
the
debug-untangle-2
branch
2 times, most recently
from
c21e7da
to
6f51279
Compare
- fix `DEBUGBUILD` guards that should be `UNITTESTS`. - fix guards for libcurl functions used in unit tests only. - sync `UNITTEST` attribute between declarations with definitions. - drop `DEBUGBUILD` guard from test `unit2600`.
- fix guards for libcurl code used by both unit tests (`unit1660`) and `test0446`.
vszakats
force-pushed
the
debug-untangle-2
branch
2 times, most recently
from
0081cb3
to
9ccdca8
Compare
Before this patch, building unit tests (= the `testdeps` target) required `-DCURLDEBUG` be set either via `ENABLE_DEBUG=ON` or `ENABLE_CURLDEBUG=ON`. After fixing the build issues in curl#13694, the above requirement is no longer necessary as a workaround. This patch makes unit tests build unconditionally. Depends-on: curl#13694 (fix build issues) Depends-on: curl#13697 (fix unit test issue revealed by Old Linux CI job) Follow-up to 39e7c22 curl#11446 Closes curl#13698
Before this patch, the `testdeps` build target required `-DCURLDEBUG` be set either via `ENABLE_DEBUG=ON` or `ENABLE_CURLDEBUG=ON` to build the curl unit tests. After fixing build issues in #13694, we can drop this requirement and build unit tests unconditionally. Depends-on: #13694 Depends-on: #13697 (fix unit test issue revealed by Old Linux CI job) Follow-up to 39e7c22 #11446 Closes #13698
…uilds
It affected cmake-unity shared-curltool curldebug mingw-w64 gcc builds
when building the `testdeps` target.
Apply the solution already used in `lib/base64.c` and `lib/dynbuf.c`
to fix it.
Also update an existing GHA CI job to test the issue fixed.
```
In file included from curl/lib/version_win32.c:35,
from curl/_bld/src/CMakeFiles/curl.dir/Unity/unity_0_c.c:145:
curl/lib/memdebug.h:52:14: error: redundant redeclaration of 'curl_dbg_logfile' [-Werror=redundant-decls]
52 | extern FILE *curl_dbg_logfile;
| ^~~~~~~~~~~~~~~~
In file included from curl/src/slist_wc.c:32,
from curl/_bld/src/CMakeFiles/curl.dir/Unity/unity_0_c.c:4:
curl/lib/memdebug.h:52:14: note: previous declaration of 'curl_dbg_logfile' with type 'FILE *' {aka 'struct _iobuf *'}
52 | extern FILE *curl_dbg_logfile;
| ^~~~~~~~~~~~~~~~
curl/lib/memdebug.h:55:44: error: redundant redeclaration of 'curl_dbg_malloc' [-Werror=redundant-decls]
55 | CURL_EXTERN ALLOC_FUNC ALLOC_SIZE(1) void *curl_dbg_malloc(size_t size,
| ^~~~~~~~~~~~~~~
curl/lib/memdebug.h:55:44: note: previous declaration of 'curl_dbg_malloc' with type 'void *(size_t, int, const char *)' {aka 'void *(long long unsigned int, int, const char *)'}
55 | CURL_EXTERN ALLOC_FUNC ALLOC_SIZE(1) void *curl_dbg_malloc(size_t size,
| ^~~~~~~~~~~~~~~
[...]
curl/lib/memdebug.h:110:17: error: redundant redeclaration of 'curl_dbg_fclose' [-Werror=redundant-decls]
110 | CURL_EXTERN int curl_dbg_fclose(FILE *file, int line, const char *source);
| ^~~~~~~~~~~~~~~
curl/lib/memdebug.h:110:17: note: previous declaration of 'curl_dbg_fclose' with type 'int(FILE *, int, const char *)' {aka 'int(struct _iobuf *, int, const char *)'}
110 | CURL_EXTERN int curl_dbg_fclose(FILE *file, int line, const char *source);
| ^~~~~~~~~~~~~~~
```
Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49840554/job/a4aoet17e9qnqx1a#L362
After: https://ci.appveyor.com/project/curlorg/curl/builds/49843735/job/hbo2uah2vj0ns523
Ref: #13689 (CI testing this PR with `DEBUGBUILD`/`CURLDEBUG`/shared-static combinations)
Depends-on: #13694
Depends-on: #13800
Closes #13705
`CURLDEBUG` is meant to enable memory tracking, but in a bunch of cases, it was protecting debug features that were supposed to be guarded with `DEBUGBUILD`. Replace these uses with `DEBUGBUILD`. This leaves `CURLDEBUG` uses solely for its intended purpose: to enable the memory tracking debug feature. Also: - autotools: rely on `DEBUGBUILD` to enable `checksrc`. Instead of `CURLDEBUG`, which worked in most cases because debug builds enable `CURLDEBUG` by default, but it's not accurate. - include `lib/easyif.h` instead of keeping a copy of a declaration. - add CI test jobs for the build issues discovered. Ref: #13694 (comment) Closes #13718
DEBUGBUILDguards that should beUNITTESTS, in libcurlcode used by unit tests.
UNITTESTattribute between declarations and definitions.DEBUGBUILDguard from testunit2600.unit1660)and
test0446.This fixes building tests with
CURLDEBUGenabled butDEBUGBUILDdisabled. This can happen when building tests with CMake with
ENABLE_DEBUG=ONin Release config, or withENABLE_CURLDEBUG=ONand without
ENABLE_DEBUG=ON. Possibly also with autotoolswhen using
--enable-curldebugwithout--enable-debug.Test results:
(the two failures are unrelated, subject to PR cmake: fix
-Wredundant-declsin unity/mingw-w64/gcc/curldebug/DLL builds #13705)Ref: #13592 (issue discovery)
Ref: #13689 (CI testing this PR with
DEBUGBUILD/CURLDEBUGcombinations)Closes #13694